chore: find reverted gmp transactions in live mode #12311
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
2 times, most recently
from
f04b63d to
28b2ce9
Compare
rabi-siddique
changed the title
Contributor
Author
|
Logs from earlier e2e testing specifically if tx is made by the Wallet Owner: Logs when tx is made non-owner of the Wallet: I’ve made many updates since the last round of testing, so I will need to do E2E testing again. |
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
5bfdeab to
51dd6d0
Compare
rabi-siddique
force-pushed
the
rs-publish-src-addr
branch
from
74778d6 to
5d58219
Compare
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
b9fde86 to
b8b1099
Compare
Contributor
Author
|
E2E testing results Success Case
Revert Case(Authorized User)
|
mergify bot
added a commit
that referenced
this pull request
Jan 12, 2026
…12327) closes: - https://github.com/Agoric/agoric-private/issues/699 ## Description Publishes the expected `sourceAddress` (LCA) for pending GMP transactions into vstorage and updates snapshots accordingly. This enables the GMP watcher to reliably classify reverted Wallet executions by comparing the decoded `sourceAddress` from `calldata` against the expected value, avoiding revert simulation and state-drift issues(see #12311). ### Testing Considerations The vstorage snapshot tests are enough to verify this change. ### Upgrade Considerations - Will require a `ymax0` and `ymax1` update on mainnet.
|
Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label. |
Contributor
Author
Revert Case(Non-Authorized)
|
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
b8b1099 to
183e078
Compare
Muneeb147
reviewed
Jan 12, 2026
Comment on lines
+269
to
+276
| if (result.settled) { | ||
| const reason = `${logPrefix} Live mode completed`; | ||
| log(reason); | ||
| abortController.abort(reason); | ||
| } |
Contributor
There was a problem hiding this comment.
What about handling tx not settled here?
Comment on lines
216
to
231
| return { settled: false }; | ||
| } | ||
|
|
||
| deleteTxBlockLowerBound(kvStore, txId); | ||
| return { found: true, txHash: matchingEvent.transactionHash }; | ||
| return { settled: true, txHash: matchingEvent.transactionHash }; | ||
| } catch (error) { | ||
| log(`Error:`, error); | ||
| return { found: false }; | ||
| return { settled: false }; |
Contributor
There was a problem hiding this comment.
do we need some extra identifier to differentiate b/w not found and error? Because both says settled: false
| `Watching for MulticallStatus and MulticallExecuted events for txId: ${txId} at contract: ${contractAddress}`, | ||
| ); | ||
| // Named so we can remove it | ||
| const onAbort = () => finish({ settled: false }); |
Contributor
There was a problem hiding this comment.
Do we wanna explicitly log when we aborted signal? Might help in debugging.
Comment on lines
190
to
193
| ws.off('message', messageHandler); | ||
| ws.off('error', onWsError); | ||
| ws.off('close', onWsClose); | ||
| signal?.removeEventListener('abort', onAbort); |
Contributor
There was a problem hiding this comment.
Add a log say: "Cleaning up ws and listeners"..?
Comment on lines
334
to
358
| if (!done) { | ||
| log( | ||
| `✗ No MulticallStatus or MulticallExecuted found for txId ${txId} within ${timeoutMs / 60000} minutes`, | ||
| ); | ||
| const { status, errorMessage } = await findTxStatusFromAxelarscan( | ||
| txId, | ||
| contractAddress, | ||
| { | ||
| axelarApiUrl, | ||
| fetch, | ||
| }, | ||
| `✗ No transaction status found for txId ${txId} within ${ | ||
| timeoutMs / 60000 | ||
| } minutes`, |
Contributor
There was a problem hiding this comment.
I see we make done = true on failure as well. So done essentially means that tx is processed? Either success/failed?
Contributor
Author
There was a problem hiding this comment.
No, done means the watcher is finished (stopped watching), regardless of whether the transaction succeeded, failed, or was never observed.
gibson042
reviewed
Jan 13, 2026
Comment on lines
276
to
319
| if (receipt.status === 1 && matchingLog) { | ||
| log( | ||
| `✅ SUCCESS: txId=${txId} txHash=${txHash} block=${receipt.blockNumber}`, | ||
| ); | ||
| return finish({ settled: true, txHash }); | ||
| } | ||
|
|
||
| if (receipt.status === 0) { | ||
| /** | ||
| * Transaction reverted - since we've already validated that the sourceAddress | ||
| * matches our expected LCA address, this is a legitimate execution attempt | ||
| * from our own wallet that failed. We treat this as a transaction failure. | ||
| * | ||
| * Note: Spurious executions from unauthorized parties are already filtered | ||
| * out by the sourceAddress check above, so any revert we see here represents | ||
| * a genuine failure of the user's operation. | ||
| */ | ||
| log( | ||
| `❌ REVERTED: txId=${txId} txHash=${txHash} block=${receipt.blockNumber} - transaction failed`, | ||
| ); | ||
| return finish({ settled: true, txHash }); | ||
| } |
Member
There was a problem hiding this comment.
I find it troubling that these two cases collapse into indistinguishable output.
Contributor
Author
There was a problem hiding this comment.
Thanks for commenting on this. Looks like I messed it up while defining settled field in all watchers.
Comment on lines
309
to
318
| subId = await provider.send('eth_subscribe', [ | ||
| 'alchemy_minedTransactions', | ||
| { | ||
| addresses: [{ to: contractAddress }], | ||
| includeRemoved: false, | ||
| hashesOnly: false, | ||
| }, | ||
| ]); | ||
|
|
||
| ws.on('message', messageHandler); |
Member
There was a problem hiding this comment.
Is there any chance of a race here? Our general pattern is to register listeners before creating circumstances that could cause them to be invoked.
Contributor
Author
There was a problem hiding this comment.
Fixed to attach the message handler before subscribing.
For additional context: at the contract level, we also follow this pattern - we publish the pending tx entry to vstorage before making the GMP call (see pos-gmp.flows.ts#L540-L566), which gives the watcher 50s-3min to start up before the transaction could complete. So we have protection at both levels.
Comment on lines
321
to
324
| // Attach listeners (all removable) | ||
| ws.on('error', onWsError); | ||
| ws.on('close', onWsClose); | ||
| signal?.addEventListener('abort', onAbort); |
Contributor
Author
There was a problem hiding this comment.
Yes, same issue. Fixed to attach the error/close listeners immediately after defining them
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
09dd75a to
edd72dd
Compare
Contributor
Author
|
Things are working as expected: Portfolio64 logs:Logs for
|
frazarshad
approved these changes
Jan 23, 2026
Contributor
There was a problem hiding this comment.
other than tests, this file isnt in use. it would make sense to probably remove it entirely and remove tests related to it as well
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
e4e2e05 to
4047914
Compare
rabi-siddique
force-pushed
the
rs-find-reverted-transactions
branch
from
4047914 to
997315d
Compare
rabi-siddique
added
automerge:rebase
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes:
Description
Removes the dependency on AxelarScan for GMP transaction failure status detection. Instead, it now uses Alchemy WebSocket connections to detect failed GMP transactions on EVM. Alchemy WebSockets are used for both failure and success detection.
This functionality is currently implemented in the live mode of the GMP watcher. Support for lookback mode will be added in a subsequent PR, as detecting reverted transactions in historical logs requires a different mechanism.
Testing Considerations
Existing tests have been adapted to the new changes. Mocks were updated as well. And couple of new tests were added for testing revert behavior.
Upgrade Considerations